fix(connect): hard-fail OAuth callback when Redis unavailable by hariom888 · Pull Request #478 · Dev-Card/DevCard

hariom888 · 2026-06-05T10:44:56Z

Summary

Fixes #438.

/connect/github/callback guarded its CSRF nonce check behind if (app.redis && ...). A Redis outage silently removed the entire guard while still allowing the OAuth exchange to proceed — with userId taken directly from the attacker-controlled base64 state parameter. An attacker who could predict or observe the state format could store a token under any arbitrary user's account.

Root cause

// Before — Redis absence silently skips the entire CSRF check
const storedUserId = app.redis
  ? await app.redis.get(`oauth:nonce:${decodedState.nonce}`)
  : null;

if (app.redis && (!storedUserId || storedUserId !== decodedState.userId)) {
  // ...reject
}
// Redis down → both branches skipped → falls through to token exchange

Fix

Hard-fail immediately if Redis is not ready. The callback cannot safely proceed without nonce verification.

// After — explicit 503 gate before any token exchange
if (!app.redis || app.redis.status !== 'ready') {
  return reply.status(503).send({ error: 'Service temporarily unavailable. Please try again.' });
}

const storedUserId = await app.redis.get(`oauth:nonce:${decodedState.nonce}`);
if (!storedUserId || storedUserId !== decodedState.userId) {
  return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=invalid_state`);
}

await app.redis.del(`oauth:nonce:${decodedState.nonce}`);

This mirrors the strictness of the initiator (GET /github), which already unconditionally calls app.redis.set — making Redis a hard dependency for both ends of the flow.

Test coverage

Scenario	Expected
Redis status !== 'ready'	503, no token written
`app.redis` is null	503, no token written
Crafted state, nonce never issued	302 → `invalid_state`
Nonce exists but userId mismatch	302 → `invalid_state`
Valid round-trip	302 → `connected=github`, nonce consumed
Nonce replay (second request)	302 → `invalid_state`
Missing code or state	302 → `missing_params`
Malformed base64 state	302 → `connect_failed`

Files changed

apps/backend/src/routes/connect.ts
apps/backend/src/__tests__/connect.test.ts

The CSRF nonce check in /connect/github/callback was guarded by if (app.redis && ...), meaning a Redis outage silently bypassed all CSRF verification. An attacker could craft a base64 state for any userId, submit a stolen OAuth code to the callback, and have a github_follow token stored under an arbitrary user's account. Changes: - Replace the conditional CSRF guard with an explicit hard-fail: if !app.redis || app.redis.status !== 'ready', return 503 immediately. The callback never reaches token exchange or DB writes without a verified nonce. - Remove all �pp.redis ? ... : null ternaries from the callback path — every Redis access is now unconditional, matching the initiator's strictness. - Nonce deletion ( edis.del) is now unconditional after a successful verification, preventing replay attacks. Tests added (connect.test.ts): - Redis unavailable (status !== 'ready') → 503, no upsert - Redis is null/falsy → 503, no upsert - Crafted state with unknown nonce → invalid_state redirect, no upsert - Nonce present but userId mismatch → invalid_state redirect, no upsert - Valid round-trip → connected=github redirect, nonce consumed - Nonce replay → second request rejected after first consumes the nonce - Missing code/state params → missing_params redirect - Malformed base64 state → connect_failed redirect

vercel · 2026-06-05T10:45:01Z

@hariom888 is attempting to deploy a commit to the Prashantkumar Khatri's projects Team on Vercel.

A member of the Team first needs to authorize it.

github-actions · 2026-06-05T10:45:44Z

CI — Checks Failed

Backend — FAIL

Check	Result
Lint	FAIL
Test	FAIL
Typecheck	FAIL

Mobile — SKIP

Check	Result
Lint	-
Test	-

Web — FAIL

Check	Result
Check	FAIL
Build	PASS

Last updated: Thu, 11 Jun 2026 11:38:36 GMT

…test.ts

hariom888 · 2026-06-05T11:17:15Z

@Harxhit

Typecheck failure is pre-existing on `main` — not introduced by this PR

All typecheck errors are in files I did not touch:

src/routes/team.ts — 6 errors (missing TeamRole export, implicit any, PrismaClientKnownRequestError)
src/__tests__/team.test.ts — 1 error (missing TeamRole export)
src/services/publicService.ts — 1 error (implicit any)
src/utils/error.util.ts — 7 errors (PrismaClientKnownRequestError, PrismaClientValidationError, unknown type)

To confirm, run tsc --noEmit on main directly — the same 15 errors appear before any changes from this PR.

Lint: ✅ PASS
Test: ✅ PASS
Typecheck: ❌ pre-existing failure unrelated to this PR

Harxhit

Fix lint error in connect.ts file.

Signed-off-by: hariom888 <hariom880088@gmail.com>

…ations

hariom888 · 2026-06-11T11:44:21Z

@Harxhit

All remaining CI failures are pre-existing on main — none are in files modified by this PR.

Lint: src/__tests__/logout.test.ts does not exist in the repo — the CI workflow references a file that was never created. Not introduced here.

Test: app.test.ts fails because JWT_SECRET and ENCRYPTION_KEY are not set in the CI environment. Not introduced here.

Typecheck: All 14 errors are in team.ts, team.test.ts, publicService.ts, and error.util.ts — none of which are touched by this PR. This matches the pre-existing failures I flagged in my earlier comment.

connect.test.ts: ✅ 8/8 tests pass.

hariom888 added 5 commits June 5, 2026 16:25

fix(lint): fix import group ordering in connect.ts and connect.test.ts

658664e

fix(lint): fix import group ordering in connect.test.ts

ed0ef6c

fix(lint): fix import group ordering in connect.test.ts

693cd04

fix(lint): fix import group ordering in connect.test.ts

a742d93

fix(lint): consolidate external imports into single group in connect.…

d739e02

…test.ts

Harxhit added the gssoc:approved Required label for every approved PR. Gives the base +50 points and enables contribution tracking. label Jun 6, 2026

ShantKhatri requested a review from Harxhit June 6, 2026 17:43

Harxhit reviewed Jun 7, 2026

View reviewed changes

hariom888 and others added 3 commits June 11, 2026 16:43

Merge branch 'main' into fix/438-oauth-csrf-bypass-redis-unavailable

06860fa

Signed-off-by: hariom888 <hariom880088@gmail.com>

fix(test): replace connect test file with non-colliding scoped declar…

327d141

…ations

fix(test): replace connect test file with non-colliding scoped declar…

88ddab0

…ations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(connect): hard-fail OAuth callback when Redis unavailable #478

fix(connect): hard-fail OAuth callback when Redis unavailable #478
hariom888 wants to merge 9 commits into
Dev-Card:mainfrom
hariom888:fix/438-oauth-csrf-bypass-redis-unavailable

hariom888 commented Jun 5, 2026

Uh oh!

vercel Bot commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

hariom888 commented Jun 5, 2026

Uh oh!

Harxhit left a comment

Uh oh!

hariom888 commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hariom888 commented Jun 5, 2026

Summary

Root cause

Fix

Test coverage

Files changed

Uh oh!

vercel Bot commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI — Checks Failed

Backend — FAIL

Mobile — SKIP

Web — FAIL

Uh oh!

hariom888 commented Jun 5, 2026

Typecheck failure is pre-existing on main — not introduced by this PR

Uh oh!

Harxhit left a comment

Choose a reason for hiding this comment

Uh oh!

hariom888 commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jun 5, 2026 •

edited

Loading

Typecheck failure is pre-existing on `main` — not introduced by this PR